Fix typeintersect bug reported in #36443
#46446
Conversation
N5N3
force-pushed
the
Fix-36443
branch
17 times, most recently
from
e23f9e5
to
9e1e4ed
Compare
This comment was marked as outdated.
This comment was marked as outdated.
N5N3
force-pushed
the
Fix-36443
branch
2 times, most recently
from
9d30a62
to
851662d
Compare
src/subtype.c
Outdated
| @@ -2877,7 +2877,7 @@ static jl_value_t *intersect_invariant(jl_value_t *x, jl_value_t *y, jl_stenv_t | |||
| jl_value_t *ii = intersect(x, y, e, 2); | |||
| e->invdepth--; | |||
| e->Rinvdepth--; | |||
| if (jl_is_typevar(x) && jl_is_typevar(y) && (jl_is_typevar(ii) || !jl_is_type(ii))) | |||
| if ((jl_is_typevar(x) || jl_is_typevar(y)) && (jl_is_typevar(ii) || !jl_is_type(ii))) | |||
There was a problem hiding this comment.
can you comment on this line change? I don't see directly see how it was related to the commit message
There was a problem hiding this comment.
IIRC, this was added as the subtype check below would ignore the offset within var_gt.
Some tests added in this PR need this change to pass. (I gave up to change var_gt.)
But yes, maybe we should not widen this branch that much. I'll try to tighten it and split this change into a seperate commit.
src/subtype.c
Outdated
| v = jl_box_long(iv + offset); | ||
| bb->lb = bb->ub = v; | ||
| // Here we always return the shorter `Vararg`'s length. | ||
| if (offset > 0) return jl_box_long(iv); |
There was a problem hiding this comment.
| v = jl_box_long(iv + offset); | |
| bb->lb = bb->ub = v; | |
| // Here we always return the shorter `Vararg`'s length. | |
| if (offset > 0) return jl_box_long(iv); | |
| bb->lb = bb->ub = jl_box_long(iv + offset); | |
| // Here we always return the shorter `Vararg`'s length. | |
| if (offset < 0) | |
| v = bb->lb; |
There was a problem hiding this comment.
looks like gc test dislike this change. Revert for now.
|
Great work! I had a few minor nits that I commented about, but otherwise looks good. Let's see how it does: @nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
Good to see there are no pkgeval failures that appear related. Should be good to merge once you make the clang-sagc-subtype test happy |
`var->offset` is used to recode the length difference of 2 `Vararg`s.
But `Vararg`'s length might also be used in the type field, e.g. `Tuple{Vararg{Val{N}, N}} where {N}`, where
we should ignore `offset` when we intersect `Val{N}`.
This commit move the offset setting/erasing into `intersect_varargs`.
Co-Authored-By: Jameson Nash <vtjnash+github@gmail.com>
The type `var` might be switched during intersection. Thus previous result looks order dependent. When we intersect 2 `Vararg`s' length, we should always return the shorter one. As we has consumed the extra elements in `intersect_tuple`. Also fix JuliaLang#37257 Co-Authored-By: Jameson Nash <vtjnash+github@gmail.com>
if `offset != 0`, then these 2 var should have different value, thus it's invalid to set bounds.
If offset > 0, the correct result is `var - offset` if expressible. So an unbounded typevar should not be returned in this case as it might be a diagonal var. Since the result could be improved if `N` get fixed, set `intvalued` to 2 as a re-intersection hint.
…ll or a local type var) But `check_unsat_bound` should not be skipped. Co-Authored-By: Jameson Nash <vtjnash+github@gmail.com>
offsetfield to recode the valid length difference between 2Varargs.It is set outside
intersect_varargs, so when we check theVararg's eltype, the intersection result would be influenced when we have e.g.Vararg{Val{N},N}.The first commit move the
offsetsetting intointersect_varargsto fix the problem.set_var_to_constwill ignore theoffsetif thetypevarhas been settled, thus gives many falseUnion{}.The second commit fix it.
Varargs length, we always return the length on the right. Which cause many order dependent error. In fact we should always return the shorterVararg's length, as the extra elements has been consumed withinintersect_tuple.Varargs length, we always set them equal, which is not correct ifoffset != 0. The 4th commit fix it (Not all, as some Vararg hasNulllength, which causes Unreachable after Varargs change #39088)close #36443, close #37257. Test added.